feat(recovery): surface the attempt-preserve ref in defer notification and status - #338
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThe change records ChangesDeferred recovery visibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RecoveryFlow
participant StoryTask
participant Engine
participant Status
RecoveryFlow->>StoryTask: set or clear preserve_ref
RecoveryFlow->>Engine: defer task with recovery state
Engine->>Engine: record story-deferred and recovery note
Engine->>Status: expose preserve_ref
Status-->>Status: render text and JSON output
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR closes the gap where a deferred story's rollback parked attempt work on an
Confidence Score: 5/5Safe to merge — the change is purely additive (new fields default to None/False, schema version unchanged) and the rollback/preserve path is guarded by a well-tested clear-then-set pattern. All new state is backward-compatible: preserve_ref and preserve_partial default gracefully on old state.json files, the journal field uses an empty-string sentinel consistently across both _defer arms, and the documents.py addition is additive to the existing schema. The control flow through RecoveryFlow is correct — the clear at the top of the auto-recover arm prevents stale refs, the last-writer-wins ordering for dirty snapshots subsumes the commits branch, and the preserve_partial latch is set unconditionally in the except block so the notice never over-promises on a commits-only ref. Tests exercise ablation paths, subsumption via merge-base --is-ancestor, partial-park downgrade, legacy-dict round-trips, and E2E ATTENTION file content, leaving the edge cases well covered. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/model.py | Adds preserve_ref: str |
| src/bmad_loop/recovery_flow.py | Clears preserve_ref/preserve_partial at the top of every auto-recover arm; preserve_attempt_commits and preserve_attempt_worktree assign them in last-writer-wins order, with preserve_partial = True latched unconditionally in the snapshot-failure except block. |
| src/bmad_loop/engine.py | Adds _defer_recovery_note() and threads it into both isolated and non-isolated _defer arms; both story-deferred journal entries consistently emit preserve_ref=task.preserve_ref or "" |
| src/bmad_loop/cli.py | Text status appends [] beside the defer reason when preserve_ref is set, using lstrip() to handle the no-reason case cleanly. |
| src/bmad_loop/documents.py | JSON status document now includes preserve_ref (additive, schema stays version 1), with an inline comment noting the field is verbatim from state and not re-validated against git. |
| tests/test_recovery_flow.py | New tests cover subsumption (dirty wins over commits branch), snapshot-failure partial-flag, and the clear-on-new-rollback ablation; each asserts the exact git ref structure with rev-parse --verify. |
| tests/test_engine.py | Adds E2E tests for the ATTENTION file's ref + command on budget-exhaustion defer, the bare-notice case (nothing parked), uniform ref-family wording, and partial-park downgrade messaging. |
| tests/test_engine_worktree.py | Extends worktree tests to assert the isolated defer names both the kept-failed branch and any earlier rolled-back ref; refactors the inline bad_dev closure into a reusable wt_bad_dev() helper. |
| tests/test_cli.py | JSON and text status tests verify preserve_ref projection and bracket-notation formatting in the human-readable output. |
| tests/test_model.py | Round-trip and legacy-dict default tests for both new fields; verifies that preserve_partial survives resumption (critical for correct partial-park notice on a resumed run). |
Sequence Diagram
sequenceDiagram
participant E as Engine._defer
participant RF as RecoveryFlow
participant PC as preserve_attempt_commits
participant PW as preserve_attempt_worktree
participant T as StoryTask
participant J as Journal
participant N as gates.notify
E->>RF: rollback_or_pause(task)
RF->>T: "preserve_ref = None, preserve_partial = False"
RF->>PC: preserve_attempt_commits(task)
alt commits above baseline and ref succeeds
PC->>T: "preserve_ref = attempt-preserve/slug"
PC->>J: attempt-commits-preserved
else no commits or ref failed
PC->>J: attempt-preserve-failed
end
RF->>PW: preserve_attempt_worktree(task)
alt snapshot succeeds and tree dirty
PW->>T: "preserve_ref = refs/attempt-preserve-dirty/slug"
PW->>J: attempt-worktree-preserved
else snapshot raises GitError
PW->>T: "preserve_partial = True"
PW->>J: attempt-worktree-preserve-failed
else clean tree
note over PW: no-op
end
RF-->>E: returns
E->>J: story-deferred with preserve_ref
E->>E: _defer_recovery_note(task)
alt in-place and preserve_ref set
E-->>N: parked at ref with git merge command
else in-place and preserve_ref None
E-->>N: bare reason only
else isolated with keep_failed branch and preserve_ref
E-->>N: branch name plus earlier parked ref
end
Reviews (2): Last reviewed commit: "fix(recovery): flag a commits-only park,..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 465: Update the README description around rollback_on_failure so recovery
refs apply only to rolled-back work; when keep_failed retains failed work, the
isolated worktree remains mounted on its unit branch and is not parked. Also
update docs/FEATURES.md around line 62 to clarify that the isolated branch is
notification-only and must not be reported as preserve_ref.
In `@src/bmad_loop/recovery_flow.py`:
- Line 341: Update the recovery flow around task.preserve_ref,
snapshot_worktree, and the defer notification so a commit-only ref is never
presented as a full recovery ref when snapshotting fails. Mark preservation as
partial or suppress the full-recovery merge command on failure, while keeping
safe_reset() behavior intact, and add a regression test covering failure after
the commit ref is stored but before the full recovery state is recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e3d752c-301e-46e8-b116-54cdfa384cf4
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/cli.pysrc/bmad_loop/documents.pysrc/bmad_loop/engine.pysrc/bmad_loop/model.pysrc/bmad_loop/recovery_flow.pytests/test_cli.pytests/test_engine.pytests/test_engine_worktree.pytests/test_model.pytests/test_recovery_flow.py
Two defects found reviewing the automated feedback on #338. `preserve_attempt_worktree` catches `verify.GitError` and returns without touching `preserve_ref`, so a failed dirty snapshot leaves the commits branch standing while `safe_reset` destroys the uncommitted half anyway. The notice then offered the uniform `merge --ff-only` line for what is only half the attempt — contradicting `_defer_recovery_note`'s own docstring, which claimed the note is empty on a preservation failure. A ref name cannot carry this: a commits-only ref is complete when the tree was clean. New `StoryTask.preserve_partial` records it and downgrades the claim. `preserve_ref` is not isolation-scoped: a unit worktree's own dev-retry rollback parks on the same shared refs, so a deferred isolated unit can carry both a kept-failed branch and an earlier attempt's ref. The isolated notice named only the branch while `status --json` reported the ref — #333's own failure mode. It now names both, without a merge command: that ref is not fast-forwardable there, and offering it would land a discarded attempt on the operator's branch. The isolated `story-deferred` entry gains `preserve_ref=` to match the in-place arm. `test_worktree_defer_keeps_failed_unit`'s `preserve_ref is None` passed for the wrong reason — its script never rolls back. Its premise is now pinned. Declined from the bot findings: the README wording (the bullet never mentions `keep_failed`, and "parked" vs "stay mounted" are already disjoint) and the journal `""`-vs-`null` divergence (`rollback-auto` two lines up already writes `baseline=... or ""`; no consumer joins the two).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #333
A deferred story's rollback parks the attempt on a recovery ref, but the ref only ever reached
journal.jsonl—preserve_attempt_commitsdropped its return value, nosrc/consumer read the events, and the notification carried a bare reason. The reporter found their work withgit log --all.What changed
StoryTask.preserve_ref(model.py) — additive, round-trips, defaultsNonefor pre-upgradestate.json.RecoveryFlowsets it — cleared at the top of every auto-recover arm, then assigned bypreserve_attempt_commits(theattempt-preserve/*branch) andpreserve_attempt_worktree(therefs/attempt-preserve-dirty/*snapshot). Dirty wins last: the snapshot iscommit-tree'd parented at the attempt's HEAD (verify.py:446-449), so it already contains the branch — onegit merge --ff-only <ref>recovers the whole attempt regardless of which families fired._defermessage tail — in place:— attempt work parked at `<ref>`; recover with `git -C "<root>" merge --ff-only <ref>`; isolated:— failed work kept on branch `<branch>`whenscm.keep_failed(the unit branch is not written topreserve_ref— a live branch is not a parked snapshot). Empty when nothing was parked, so the notice can never advertise a ref that was not created. The in-placestory-deferredjournal entry gainspreserve_ref=.documents.py(--json, additive; schema stays 1),cmd_statustext (<reason> [<ref>]), README + FEATURES.The escalation path is untouched — it does not roll back, so nothing is parked there. No policy or
core.tomlchanges.Caveat, documented not enforced
preserve_refis reported verbatim fromstate.json.scm.preserve_keepprunes the oldest refs at a later run's start, so a named ref may be gone.statusmust not run git, so this is a README/docstring note rather than a validation.Tests
Both ablations run before trusting the negative assertions:
task.preserve_ref = Nonefrom the auto-recover arm →test_rollback_clears_a_previous_attempts_preserve_reffails (assert 'attempt-preserve/run-1-97d596da' is None).test_defer_notification_stays_bare_when_nothing_was_parkedandtest_defer_recovery_note_is_uniform_across_ref_familiesboth fail (merge --ff-only None).Also added: both-families subsumption (
git merge-base --is-ancestor branch dirty), engine E2E asserting the ATTENTION file's ref + command on a budget-exhaustion defer, worktree defer with/withoutkeep_failed, model round-trip + legacy-dict default,statusjson + text.Note on the first draft of ablation 2: the bare-reason case was initially written against a
pre_ready_gateplugin veto, which turned out to have its own notify site (engine.py:1002-1003) and never reaches_defer— the test passed under ablation. Rewritten against over-budget sessions, which touch no files, so the exhaustion defer's rollback finds a clean tree and genuinely parks nothing.uv run pytest -q -n auto: 3360 passed, 24 skipped.trunk check: clean.Summary by CodeRabbit
git ... merge --ff-onlyrestore guidance, including special wording for worktree isolation and commits-only preservation.bmad-loop status(text and--json) now includespreserve_reffor deferred tasks, and status lines visually annotate it when set.preserve_refpersistence, clearing, and notification/status output in rollback and worktree scenarios.